Replace reqwest with http and hyper.
#294
Conversation
| pub fn base_url(mut self, base_url: impl reqwest::IntoUrl) -> Result<Self> { | ||
| self.base_url = Some(base_url.into_url().context(crate::error::HttpSnafu)?); |
There was a problem hiding this comment.
This should use TryInto<Url> instead of AsRef, because then we can still accept Url in addition to &str.
There was a problem hiding this comment.
Looking at the docs: https://doc.rust-lang.org/std/convert/trait.TryInto.html
Library authors should usually not directly implement this trait, but should prefer implementing the TryFrom trait, which offers greater flexibility and provides an equivalent TryInto implementation for free, thanks to a blanket implementation in the standard library.
There was a problem hiding this comment.
Yes, that documentation is for implementing TryInto (e.g. impl TryInto<Url> for str {}), you don't implement TryInto, because all TryFrom implementations automatically implement TryInto, but you can still use TryInto where appropriate, otherwise the traits wouldn't be very useful. 🙂
There was a problem hiding this comment.
but you can still use TryInto where appropriate
Can you tell the syntax to this please?
There was a problem hiding this comment.
It's the same as what you did for AsRef<str>.
fn foo(url: impl TryInto<Url>) -> Result<()> {
let url = url.try_into()?;
Ok(())
}There was a problem hiding this comment.
I tried to use it, however I could not get the types to match up.
|
What a coincidence, we were actually just getting started with implementing tower/hyper/http for octocrab. I will likely make a separate PR for this, and we can compare/merge best of both PRs |
|
@L1ghtman2k If you'd like, you can edit this PR. I don't think I'll be able to do the hyper integration; it's too complicated for me. I'm more than happy to add you as a contributor to my fork. |
Hey, it is definitely ambitious, I am myself am relatively new to rust, will see how far I can get. No need to give access, I have already cherry picked some of your changes |
|
The hyper integration should be straightforward, it's replacing |
|
@L1ghtman2k Any progress made on this? |
Yes, I am working on it here: #297 (still need to wrap few things before it is ready for review) |
|
Awesome! Closing this in favor of #297. |
Fixes #99.